feat(ui): invitation flow smart routing#10589
Conversation
|
✅ Conflict Markers Resolved All conflict markers have been successfully resolved in this pull request. |
|
✅ All necessary |
🔒 Container Security ScanImage: ✅ No Vulnerabilities DetectedThe container image passed all security checks. No known CVEs were found.📋 Resources:
|
alejandrobailo
left a comment
There was a problem hiding this comment.
Code review
Found 2 issues:
useCallbackis not allowed per AGENTS.md CRITICAL RULES ("NEVER:useMemo,useCallback— React Compiler handles optimization").doAcceptcan be a plainasync functioninside the component, the compiler will take care of referential stability.
prowler/ui/app/(auth)/invitation/accept/accept-invitation-client.tsx
Lines 63 to 75 in 9ae299a
isInvitationPageinauth.config.tsis too broad. It usesstartsWith("/invitation"), which lets any path under/invitation/*bypass auth in the NextAuth layer.proxy.tswas already narrowed to/invitation/acceptonly (commit "restrict public route to /invitation/accept only"), butauth.config.tswas missed. Should be:
const isInvitationPage = nextUrl.pathname.startsWith("/invitation/accept");Line 284 in 9ae299a
Overall the architecture is clean, nice work splitting the (guest-only) layout and the backward-compat redirect with the action=signup escape hatch. Just these two to tidy up 👍
Co-authored-by: Pablo F.G <pablo.fernandez@prowler.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…298) The backward-compatibility redirect in proxy.ts was catching the sign-up navigation from the smart router, sending the user back to /invitation/accept in an infinite loop. Adding an `action=signup` query param lets the middleware distinguish smart-router navigations from legacy invitation links. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Narrow the public route from /invitation to /invitation/accept to follow the principle of least privilege. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Next.js 16 requires a Suspense boundary for pages using dynamic searchParams to allow static generation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The real cause of the prerender error was NavigationProgress in the (auth) layout using useSearchParams without a Suspense boundary. Other (auth) pages didn't hit this because the (guest-only) layout calls auth() which makes them dynamic. /invitation/accept sits directly under (auth) so Next.js tried to prerender it and failed. Reverts the force-dynamic and Suspense workarounds from page.tsx. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…Url (PROWLER-1298) The middleware now correctly preserves query parameters in callbackUrl when redirecting to sign-in. Update the test assertion to match the fixed behavior. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Remove useCallback from doAccept (React Compiler handles optimization) - Restrict isInvitationPage to /invitation/accept only in auth.config.ts Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
9ae299a to
837a84f
Compare
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Alan-TheGentleman
left a comment
There was a problem hiding this comment.
Seven things to address. The first three I'd consider blocking, the rest are smaller but worth doing in the same PR.
-
/sign-inbutton in the 404 error state is a dead end for authenticated users. When the API returns 404 (wrong-email case), the user is logged in with the wrong account and clicks "Go to Sign In" — but/sign-innow lives under(guest-only)layout, which redirects authenticated users back to/. They land on the wrong-account dashboard with no recovery path. Fix: gate on the 404 case and sign out before redirect:await signOut({ redirect: false }); router.push("/sign-in?callbackUrl=" + encodeURIComponent(callbackPath)). -
Error mapping by HTTP status is fragile. Switching on
400/404/410assumes those codes only ever mean "already used", "wrong email", "expired" — but the API will inevitably return 400 for malformed tokens and 404 for nonexistent invitations too, both of which will surface the wrong message.handleApiResponsealready exposeserrors[0]from the JSON:API body — switch onresult.errors?.[0]?.codeand keepstatusonly as a fallback. If the API doesn't have stable error codes yet, open a follow-up. -
E2E coverage is too thin for an auth flow refactor. The new test only validates query param preservation in
callbackUrl. Missing: authenticated happy path (token + auth → accept → dashboard), unauthenticatedchoose→ sign-in → back to accept, the three error states (410/400/404), the backward-compat/sign-up?invitation_token=redirect, and the(guest-only)redirect of authenticated users.ui/CLAUDE.mdmarkstddas mandatory for refactors — add at least the four flow tests or link a follow-up ticket here before merge. -
acceptInvitationserver action accepts unvalidated input. The token comes from a URL search param (attacker-controllable) and goes straight toJSON.stringifywithout Zod validation. The convention inui/CLAUDE.mdis all inputs validated with Zod. Addz.object({ token: z.string().min(1).max(500) })at the entry of the action. -
action=signupis a magic string contract betweenproxy.tsand the client. The bypass works because both files happen to use the literal"signup", but there's no shared definition — six months from now someone refactors the middleware, sees the unexplainedactionparam, and removes it, silently breaking the smart-router-to-sign-up path. Extract toui/lib/invitation-routing.tsasexport const INVITATION_SIGNUP_ACTION = "signup"and import on both sides. -
kind: "loading"is dead state. TheuseStatelazy initializer sets it for the auth+token case, but theuseEffectimmediately overwrites it withkind: "accepting"— both render the same spinner. Droploadingfrom the union and start inaccepting. Better still: move the auth+token decision intopage.tsx(server component) so the happy path doesn't flash a client loading state at all. -
setState({ kind: "success" }) + router.push("/")race. The success UI may or may not flash for a tick depending on timing. Either drop the success state and navigate immediately, or keep it visible with an 800–1000ms delay before navigation.
What's good: the (guest-only) route group is the right Next.js 15 pattern, the discriminated union on state instead of boolean flags, the backward-compat in proxy.ts thinking about users with old links in their inbox, and the callbackUrl + nextUrl.search fix is the correct shape with an E2E test backing it.
- Sign out before redirect on 404 error (wrong email) to avoid dead end - Remove dead states: "loading" and "success" from AcceptState union - Add Zod validation to acceptInvitation server action - Extract shared INVITATION_ACTION_PARAM/SIGNUP_ACTION constants - Follow-ups: error codes (#10612), E2E coverage (#10613) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Alan-TheGentleman
left a comment
There was a problem hiding this comment.
Re-reviewed after f742f6c. Five of seven items addressed cleanly: the sign-out dead end is fixed with needsSignOut + handleSignOutAndRedirect, Zod validation is in, magic strings extracted to invitation-routing.ts with a good JSDoc, and both dead states (loading + success) are gone — the union is now just no-token | accepting | error | choose.
Two remaining items are acceptable as follow-ups:
-
Error mapping still switches on HTTP status. When the API adds stable error codes to the JSON:API errors array, switch to result.errors?.[0]?.code. Until then the current mapping works, just fragile.
-
E2E coverage. The auth flow refactor has one new test (query param preservation) but no coverage of the smart router states, backward-compat redirect, or guest-only layout redirect. Worth a follow-up PR or ticket to close the TDD gap before the next round of changes to this flow.
Clean iteration — good turnaround.
Context
Implements the invitation flow smart routing for PROWLER-1298. When a user clicks an invitation link, instead of being sent directly to sign-up, they are now routed through a smart router that detects their authentication state and guides them
accordingly (sign in, sign up, or auto-accept).
Fix PROWLER-1298
Description
This PR introduces a new
/invitation/acceptroute that acts as a smart router for invitation links. The key changes are:acceptInvitationserver action (ui/actions/invitations/invitation.ts): CallsPOST /invitations/acceptwith the invitation token./invitation/acceptpage and client component (ui/app/(auth)/invitation/accept/): A smart router that handles three scenarios:ui/app/(auth)/(guest-only)/layout.tsx): Movedsign-inandsign-uppages under a new(guest-only)route group that redirects authenticated users to/. The parent(auth)layout no longer performs thisredirect, allowing the invitation accept page to work for both authenticated and unauthenticated users.
ui/components/invitations/invitation-details.tsx): Generated invitation links now point to/invitation/accept?invitation_token=...instead of/sign-up?invitation_token=....ui/proxy.ts): Old/sign-up?invitation_token=...links are automatically redirected to the new/invitation/acceptroute.ui/auth.config.ts,ui/proxy.ts):callbackUrlnow includesnextUrl.searchso query parameters (likeinvitation_token) survive the sign-in redirect flow.ui/tests/auth/auth-session-errors.spec.ts): Validates that query parameters are preserved in thecallbackUrlduring redirects.Steps to review
ui/app/(auth)/invitation/accept/accept-invitation-client.tsx— verify that all state transitions (loading → accepting → success/error,choose → sign-in/sign-up) are correct.(guest-only)route group correctly restrictssign-in/sign-upto unauthenticated users while leaving/invitation/acceptaccessible to both./sign-up?invitation_token=...) should redirect to/invitation/accept?invitation_token=...via the proxy middleware.callbackUrlnow preserves query parameters in bothauth.config.tsandproxy.ts.mapApiErrorfor correct HTTP status code handling (410, 400, 404).npx playwright test ui/tests/auth/auth-session-errors.spec.ts.Checklist
Community Checklist
SDK/CLI
UI
API
N/A — No API changes in this PR.
License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.